Skip to content
This repository has been archived by the owner on Feb 21, 2023. It is now read-only.

Support for async callbacks #1284

Merged
merged 6 commits into from
Feb 4, 2022
Merged

Support for async callbacks #1284

merged 6 commits into from
Feb 4, 2022

Conversation

stephanm
Copy link

@stephanm stephanm commented Feb 2, 2022 •
edited
Loading

What do these changes do?

As mentioned in #1235 you can't use coroutines as custom callbacks. This patch tries to fix that.

Are there changes in behavior for the user?

There shouldn't be any changes for the users as sync callbacks are still supported.

Related issue number

#1235

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES/ folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example:
      Fix issue with non-ascii contents in doctest text files.

Andrew-Chen-Wang
Andrew-Chen-Wang previously requested changes Feb 2, 2022
View reviewed changes
Copy link
Collaborator

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test where you receive two callbacks, one is sync and the other an async func? Plus lint with precommit. Thanks a ton! Also, pertaining to ordering, I can't actually remember my digging around of the code. In relation to your comment, dicts should be ordered now in supported Py3 versions, but I suppose it doesn't matter here 😅

@stephanm
Copy link
Author

stephanm commented Feb 2, 2022

Yeah, no problem, I'll add the test and have a look at the linter error.
Thanks for the clarifications and the fast feedback!
TIL: Ordering is guaranteed since 3.7 :)

@codecov
Copy link

codecov bot commented Feb 3, 2022 •
edited
Loading

Codecov Report

Merging #1284 (359a741) into master (56d6b32) will decrease coverage by 0.02%.
The diff coverage is 96.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1284      +/-   ##
==========================================
- Coverage   90.66%   90.63%   -0.03%     
==========================================
  Files          21       21              
  Lines        6870     6891      +21     
  Branches      884      885       +1     
==========================================
+ Hits         6229     6246      +17     
- Misses        469      471       +2     
- Partials      172      174       +2
Flag Coverage Δ
unit 90.55% <96.15%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aioredis/client.py 82.38% <85.71%> (-0.09%) ⬇️
tests/test_pubsub.py 99.27% <100.00%> (+0.03%) ⬆️
aioredis/connection.py 77.18% <0.00%> (-0.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56d6b32...359a741. Read the comment docs.

@stephanm
Copy link
Author

stephanm commented Feb 3, 2022

I added a test case that uses both callback variants.

The typing problem wasn't that easy to fix.
Using something like
PubSubHandler = Callable[[Dict[str, str]], Union[None, Awaitable[None]]]
doesn't work:
aioredis/client.py:4236: error: Incompatible types in "await" (actual type "Optional[Awaitable[None]]", expected type "Awaitable[Any]") [misc]
Maybe someone who is better at this typing stuff can comment on this issue? The current variant at least works with mypy :)

seandstewart
seandstewart approved these changes Feb 4, 2022
View reviewed changes
Copy link
Collaborator

@seandstewart seandstewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution here!

@seandstewart seandstewart dismissed Andrew-Chen-Wang’s stale review February 4, 2022 13:07

Tests look good to me at this point!

@seandstewart seandstewart merged commit 659a14d into aio-libs-abandoned:master Feb 4, 2022
Andrew-Chen-Wang added a commit to Andrew-Chen-Wang/redis-py that referenced this pull request Feb 12, 2022
@Andrew-Chen-Wang
Add aio-libs-abandoned/aioredis-py#1256 & aio-libs-abandoned/aioredis…
90e129c 
…-py#1284
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@seandstewart seandstewart seandstewart approved these changes

@Andrew-Chen-Wang Andrew-Chen-Wang Andrew-Chen-Wang left review comments

Assignees
No one assigned
Labels
bot:chronographer:provided
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stephanm @seandstewart @Andrew-Chen-Wang